quiche: support inplace filter chain update by danzh2010 · Pull Request #17988 · envoyproxy/envoy

danzh2010 · 2021-09-03T22:23:12Z

Commit Message: updateListenerConfig() and onFilterChainDraining() to ActiveListener interface and implement them in ActiveQuicListener. Unblock listener update for UDP listener.

Risk Level: high, touches LDS.
Testing: new integration tests
Fixes #13115

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 · 2021-09-07T17:19:48Z

/assign @lambdai @mattklein123

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 · 2021-09-08T17:22:31Z

api_compat failure is not related. This PR is ready for review!

danzh2010 · 2021-09-09T16:07:43Z

Ping?

ggreenway

This looks really good overall. Thanks!

I'd like to have @mattklein123 take a look also; he's the expert in listener updates.

/wait

source/common/quic/active_quic_listener.cc

source/common/quic/envoy_quic_dispatcher.cc

source/server/active_udp_listener.h

Signed-off-by: Dan Zhang <danzh@google.com>

ggreenway · 2021-09-13T15:49:43Z

CI is failing; looks like a real failure:

2021-09-09T22:26:06.8171992Z [ RUN      ] QuicHttpIntegrationTests/QuicInplaceLdsIntegrationTest.ReloadConfigUpdateNonDefaultFilterChain/IPv4
2021-09-09T22:26:06.8172771Z =================================================================
2021-09-09T22:26:06.8173657Z ==17==ERROR: AddressSanitizer: use-after-poison on address 0x60c000191790 at pc 0x0000177128f7 bp 0x7f02f5969730 sp 0x7f02f5969728```

/wait

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 · 2021-09-14T15:14:05Z

CI is failing; looks like a real failure:

2021-09-09T22:26:06.8171992Z [ RUN      ] QuicHttpIntegrationTests/QuicInplaceLdsIntegrationTest.ReloadConfigUpdateNonDefaultFilterChain/IPv4
2021-09-09T22:26:06.8172771Z =================================================================
2021-09-09T22:26:06.8173657Z ==17==ERROR: AddressSanitizer: use-after-poison on address 0x60c000191790 at pc 0x0000177128f7 bp 0x7f02f5969730 sp 0x7f02f5969728```

/wait

fixed

danzh2010 · 2021-09-15T15:37:41Z

/assign @mattklein123

mattklein123

I didn't review the H3 code, just the core code. Generally LGTM with a few questions/comments, thank you.

/wait

source/server/active_udp_listener.h

mattklein123 · 2021-09-15T19:36:46Z

source/server/connection_handler_impl.cc

+    if (!Runtime::runtimeFeatureEnabled(
+            "envoy.reloadable_features.udp_listener_updates_filter_chain_in_place") &&


nit: snap this runtime value above.

mattklein123 · 2021-09-15T19:38:14Z

source/server/listener_impl.cc

-          Network::Socket::Type::Stream ||
-      Network::Utility::protobufAddressSocketType(config.address()) !=
-          Network::Socket::Type::Stream) {
+  ;


mattklein123 · 2021-09-15T19:41:12Z

source/server/listener_impl.cc

-  // buildUdpListenerFactory() must come before buildListenSocketOptions() because the UDP
-  // listener factory can provide additional options.
-  buildUdpListenerFactory(socket_type, concurrency);


Just so I understand this is removed because this is the in-place update path, which was never supported before for UDP, and now that it is, we use the existing factory or just don't need a factory? Is that right?

Yes, we already setup udp_listener_config_ in the original listener and it is shared with the new listener a few lines above.

danzh2010 · 2021-09-16T22:28:21Z

/retest

repokitteh-read-only · 2021-09-16T22:28:24Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #17988 (comment) was created by @danzh2010.

see: more, trace.

mattklein123

Thanks LGTM with small remaining comments.

/wait

mattklein123 · 2021-09-17T15:22:04Z

docs/root/version_history/current.rst

 * route config: added :ref:`dynamic_metadata <envoy_v3_api_field_config.route.v3.RouteMatch.dynamic_metadata>` for routing based on dynamic metadata.
 * sxg_filter: added filter to transform response to SXG package to :ref:`contrib images <install_contrib>`. This can be enabled by setting :ref:`SXG <envoy_v3_api_msg_extensions.filters.http.sxg.v3alpha.SXG>` configuration.
 * thrift_proxy: added support for :ref:`mirroring requests <envoy_v3_api_field_extensions.filters.network.thrift_proxy.v3.RouteAction.request_mirror_policies>`.
+* udp: allows updating filter chain in-place through LDS, which is supported by Quic listener. This will be No-op with error log in other UDP listener implementations. It can be reverted by ``envoy.reloadable_features.udp_listener_updates_filter_chain_in_place``.


I think this needs to be updated now that we will reject for non-QUIC?

mattklein123 · 2021-09-17T15:23:03Z

source/server/active_udp_listener.h

+  void updateListenerConfig(Network::ListenerConfig&) override { NOT_REACHED_GCOVR_EXCL_LINE; }
+  void onFilterChainDraining(const std::list<const Network::FilterChain*>&) override {
+    NOT_REACHED_GCOVR_EXCL_LINE;
+  }


Can you add some small comments on why this is not reached?

mattklein123 · 2021-09-17T15:23:13Z

source/server/connection_handler_impl.cc


 void ConnectionHandlerImpl::addListener(absl::optional<uint64_t> overridden_listener,
                                        Network::ListenerConfig& config) {
+  bool support_udp_in_place_filter_chain_update = Runtime::runtimeFeatureEnabled(


mattklein123 · 2021-09-17T15:25:41Z

source/server/listener_impl.cc

-          Network::Socket::Type::Stream ||
-      Network::Utility::protobufAddressSocketType(config.address()) !=
-          Network::Socket::Type::Stream) {
+  if (!Runtime::runtimeFeatureEnabled(


Does the code that you added in validateFilterChains need to be replicated here? I forget the control flow. Can you add a comment on why this is OK given the other check?

No. The problematic config will be rejected later. The following newListenerWithFilterChain() will construct ListenerImpl object and throw if validateFilterChains() fails. validateFilterChains() in called in both ListenerImpl constructors so such config will be rejected no matter it's an in-place update or it's the start up.

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010

Added feature protection around filter chain validation

danzh2010 · 2021-09-17T18:26:19Z

docs/root/version_history/current.rst

 * route config: added :ref:`dynamic_metadata <envoy_v3_api_field_config.route.v3.RouteMatch.dynamic_metadata>` for routing based on dynamic metadata.
 * sxg_filter: added filter to transform response to SXG package to :ref:`contrib images <install_contrib>`. This can be enabled by setting :ref:`SXG <envoy_v3_api_msg_extensions.filters.http.sxg.v3alpha.SXG>` configuration.
 * thrift_proxy: added support for :ref:`mirroring requests <envoy_v3_api_field_extensions.filters.network.thrift_proxy.v3.RouteAction.request_mirror_policies>`.
+* udp: allows updating filter chain in-place through LDS, which is supported by Quic listener. This will be No-op with error log in other UDP listener implementations. It can be reverted by ``envoy.reloadable_features.udp_listener_updates_filter_chain_in_place``.


danzh2010 · 2021-09-17T18:27:01Z

source/server/active_udp_listener.h

+  void updateListenerConfig(Network::ListenerConfig&) override { NOT_REACHED_GCOVR_EXCL_LINE; }
+  void onFilterChainDraining(const std::list<const Network::FilterChain*>&) override {
+    NOT_REACHED_GCOVR_EXCL_LINE;
+  }


danzh2010 · 2021-09-17T18:51:10Z

source/server/connection_handler_impl.cc


 void ConnectionHandlerImpl::addListener(absl::optional<uint64_t> overridden_listener,
                                        Network::ListenerConfig& config) {
+  bool support_udp_in_place_filter_chain_update = Runtime::runtimeFeatureEnabled(


danzh2010 · 2021-09-17T18:55:48Z

source/server/listener_impl.cc

-          Network::Socket::Type::Stream ||
-      Network::Utility::protobufAddressSocketType(config.address()) !=
-          Network::Socket::Type::Stream) {
+  if (!Runtime::runtimeFeatureEnabled(


No. The problematic config will be rejected later. The following newListenerWithFilterChain() will construct ListenerImpl object and throw if validateFilterChains() fails. validateFilterChains() in called in both ListenerImpl constructors so such config will be rejected no matter it's an in-place update or it's the start up.

mattklein123

Thanks!

danzh2010 · 2021-09-20T15:15:57Z

/assign @alyssawilk for HTTP/3 side code.

repokitteh-read-only · 2021-09-20T15:16:01Z

🙀 Error while processing event:

evaluation error
error: function _rk_error error: path contains forbidden characters:
 Traceback (most recent call last):
  bootstrap:143: in <toplevel>
  bootstrap:135: in _main
  bootstrap:62: in _call
  bootstrap:15: in _call1
  github.com/repokitteh/modules/assign.star:18: in _assign
  github:395: in issue_check_assignee
  github:131: in call
Error: function _rk_error error: path contains forbidden characters

🐱

Caused by: a #17988 (comment) was created by @danzh2010.

see: more, trace.

danzh2010 · 2021-09-20T15:16:16Z

/assign @alyssawilk

danzh2010 · 2021-09-20T18:13:43Z

@ggreenway mind taking another look?

alyssawilk

Thanks for tackling this one! Couple of nits below

alyssawilk · 2021-09-20T19:19:44Z

source/server/active_tcp_listener.h


  /**
-   * Update the listener config. The follow up connections will see the new config. The existing
+   * The follow up connections will see the new config. The existing


was this comment changed intentionally? I think it made more sense before

It's by mistake. Fixed

alyssawilk · 2021-09-20T19:22:06Z

test/integration/quic_http_integration_test.cc

  ASSERT_TRUE(response->waitForReset());
 }

+class QuicInplaceLdsIntegrationTest : public QuicHttpIntegrationTest {


Can we not reuse LdsInplaceUpdateHttpIntegrationTest.ReloadConfigAddingFilterChain and friends?

Unfortunately, that test uses various ALPN to distinguish filter chains, but QUIC only supports "h3".

Gotcha. If you make any further changes can you comment as such? Or alternately if when you address the ALPN TODO you could then share code maybe TODO the test migration?

source/common/quic/envoy_quic_server_session.h

alyssawilk · 2021-09-20T19:25:46Z

source/server/listener_impl.cc

-  open_connections_ = origin.open_connections_;
+
+  if (socket_type == Network::Socket::Type::Stream) {
+    // Apply below tcp only initialization.


// Apply the options below only for TCP.

(though I'm surprised socket options doesn't apply to UDP?)

done.
buildSocketOptions() is not called for UDP in the other ListenerImpl constructor either. It initializes connection balancer and sets TCP fast open which doesn't apply to UDP.

Signed-off-by: Dan Zhang <danzh@google.com>

alyssawilk

quiche part looks good. I'd appreciate Greg or Matt signing off on the reload since they're more familiar with that.

alyssawilk · 2021-09-21T12:16:42Z

test/integration/quic_http_integration_test.cc

  ASSERT_TRUE(response->waitForReset());
 }

+class QuicInplaceLdsIntegrationTest : public QuicHttpIntegrationTest {


Gotcha. If you make any further changes can you comment as such? Or alternately if when you address the ALPN TODO you could then share code maybe TODO the test migration?

danzh2010 · 2021-09-21T15:45:13Z

/retest

repokitteh-read-only · 2021-09-21T15:45:17Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #17988 (comment) was created by @danzh2010.

see: more, trace.

danzh2010 · 2021-09-22T16:10:35Z

@ggreenway @mattklein123 any other comments?

danzh2010 · 2021-09-23T19:37:23Z

Ping @ggreenway @mattklein123

ggreenway · 2021-09-23T19:41:47Z

Apologies; I'm very busy this week. I may not be able to review until next Monday.

mattklein123

I'm OK from my side, thank you.

alyssawilk · 2021-09-23T19:54:56Z

That's 2 senior reviewers so I'm inclined to just merge if no one objects

ggreenway · 2021-09-23T20:46:54Z

That's 2 senior reviewers so I'm inclined to just merge if no one objects

I did a pass earlier, and I think all my concerns have been addressed. I vote for merging it.

addressed

danzh1989 added 7 commits September 3, 2021 15:41

inplace filter chain update

2352e5c

Signed-off-by: Dan Zhang <danzh@google.com>

revert debug change

6b469dc

Signed-off-by: Dan Zhang <danzh@google.com>

Merge branch 'master' into filterchainupdate

f091e2e

Signed-off-by: Dan Zhang <danzh@google.com>

remove udp_listener_impl change

89d0870

Signed-off-by: Dan Zhang <danzh@google.com>

remove unused includes

6d9f9c2

Signed-off-by: Dan Zhang <danzh@google.com>

remove debug log

3bd1557

Signed-off-by: Dan Zhang <danzh@google.com>

remove apln plumbing

8e5889b

Signed-off-by: Dan Zhang <danzh@google.com>

zuercher assigned ggreenway Sep 7, 2021

fix tests

164867f

Signed-off-by: Dan Zhang <danzh@google.com>

repokitteh-read-only bot assigned lambdai and mattklein123 Sep 7, 2021

danzh1989 added 2 commits September 7, 2021 19:22

fix proof source test

6bb5415

Signed-off-by: Dan Zhang <danzh@google.com>

Merge branch 'master' into filterchainupdate

4ec262c

Signed-off-by: Dan Zhang <danzh@google.com>

ggreenway previously requested changes Sep 9, 2021

View reviewed changes

repokitteh-read-only bot added the waiting label Sep 9, 2021

address comments

eb7c29c

Signed-off-by: Dan Zhang <danzh@google.com>

repokitteh-read-only bot removed the waiting label Sep 9, 2021

mattklein123 removed their assignment Sep 9, 2021

repokitteh-read-only bot added the waiting label Sep 13, 2021

fix asan

75dd2c2

Signed-off-by: Dan Zhang <danzh@google.com>

repokitteh-read-only bot removed the waiting label Sep 13, 2021

repokitteh-read-only bot assigned mattklein123 Sep 15, 2021

mattklein123 requested changes Sep 15, 2021

View reviewed changes

repokitteh-read-only bot added the waiting label Sep 15, 2021

repokitteh-read-only bot removed the waiting label Sep 16, 2021

mattklein123 requested changes Sep 17, 2021

View reviewed changes

repokitteh-read-only bot added the waiting label Sep 17, 2021

guard filter chain verification

3dcecd0

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 commented Sep 17, 2021

View reviewed changes

repokitteh-read-only bot removed the waiting label Sep 17, 2021

mattklein123 previously approved these changes Sep 17, 2021

View reviewed changes

repokitteh-read-only bot assigned alyssawilk Sep 20, 2021

danzh2010 requested a review from ggreenway September 20, 2021 18:13

alyssawilk reviewed Sep 20, 2021

View reviewed changes

comments

844c366

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 dismissed mattklein123’s stale review via 844c366 September 21, 2021 01:07

alyssawilk approved these changes Sep 21, 2021

View reviewed changes

mattklein123 approved these changes Sep 23, 2021

View reviewed changes

alyssawilk merged commit 7f93f94 into envoyproxy:main Sep 23, 2021

mattklein123 mentioned this pull request Apr 25, 2022

envoy_reloadable_features_udp_listener_updates_filter_chain_in_place deprecation #20991

Closed

		if (!Runtime::runtimeFeatureEnabled(
		"envoy.reloadable_features.udp_listener_updates_filter_chain_in_place") &&

Conversation

danzh2010 commented Sep 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danzh2010 commented Sep 7, 2021

Uh oh!

danzh2010 commented Sep 8, 2021

Uh oh!

danzh2010 commented Sep 9, 2021

Uh oh!

ggreenway left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ggreenway commented Sep 13, 2021

Uh oh!

danzh2010 commented Sep 14, 2021

Uh oh!

danzh2010 commented Sep 15, 2021

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danzh2010 commented Sep 16, 2021

Uh oh!

repokitteh-read-only bot commented Sep 16, 2021

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danzh2010 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

danzh2010 commented Sep 20, 2021

Uh oh!

danzh2010 commented Sep 3, 2021 •

edited

Loading